add embedding for all image nfts, show similar nfts in nft page, expo… - #145

Code Review — PR #145: NFT Embedding, Similar NFTs, and Export

Overview

This PR adds three related features:

1. A new Python/FastAPI microservice (microservices/semantic/) that CLIP-encodes images for semantic search
2. A SimilarNfts component + useSimilarNfts hook that surfaces semantically similar NFTs on the NFT detail page
3. An "Export" button in ShelfDetail that downloads NFT Arweave IDs as JSON

The overall approach is sound. Below are the issues I found, ordered by severity.

Bugs / Correctness Issues

useSimilarNfts.ts — N+1 canister calls (performance + correctness risk)

Each similarity hit triggers two sequential canister round-trips (getOwnerOf + tokenToIcpInfo). For TOP_K = 20, that's up to 40 canister calls fired in parallel inside a single Promise.all. This is likely to hit IC rate limits or timeout walls in production. Consider batching or accepting that some fields will be lazily loaded after the cards render (similar to how the rest of the Alexandrian NFT list works).

// Current: 20 × 2 = 40 concurrent canister calls
return Promise.all(
  hits.map(async (r) => {
    const ownerRes = await adapter.getOwnerOf([tokenId]);
    const icpInfo  = await adapter.tokenToIcpInfo(tokenId);
    ...
  }),
);

useSimilarNfts.ts — non-null assertions on actor and arweaveId

The enabled: !!actor && !!arweaveId guard is correct, but inside queryFn the code still uses arweaveId! and actor!. If React Query ever calls queryFn before the guard settles (e.g., during a race on initial mount), this will throw a confusing runtime error. Prefer an explicit guard with an early throw:

if (!actor || !arweaveId) throw new Error("actor or arweaveId not ready");

ShelfDetail.tsx — redundant filter inside the export handler

contentFilter === "Nft" already gates the button's visibility, so filteredItems already contains only NFT items. The inner .filter(([, item]) => "Nft" in item.content) is technically correct but misleading — it implies the outer filter might not be reliable. Remove the inner filter or add a comment explaining why the double-check is needed.

Security Concerns

server.py — unbounded memory for image download

requests.get(url, timeout=30) loads the entire Arweave payload into resp.content before size-checking. A malicious or misconfigured Arweave transaction could serve a very large file. The HEAD check for content-type is good but doesn't check Content-Length. Add a size cap on the GET response:

resp = requests.get(url, timeout=30, stream=True)
data = b""
for chunk in resp.iter_content(chunk_size=1 << 20):
    data += chunk
    if len(data) > MAX_UPLOAD_BYTES:
        raise HTTPException(413, "image exceeds size limit")

server.py — CORS allow_origins=["*"]

This is acceptable for a read-only embedding API, but worth an explicit comment confirming the service holds no secrets or user data, so open CORS is intentional. If the service is later extended with write endpoints, this will need to be revisited.

useSimilarNfts.ts — hardcoded fallback server URL

const EMBEDDING_SERVER =
  process.env.REACT_APP_EMBEDDING_SERVER || "https://lbry.youthumber.com";

The hardcoded fallback to lbry.youthumber.com means the frontend will silently call an external third-party server in production if the env var is missing. This is a supply-chain/trust risk. Either:

• Fail loudly when the env var is absent (throw / log a warning), or
• Document clearly who controls lbry.youthumber.com and confirm it's first-party.

Code Quality

server.py — synchronous requests inside async FastAPI handlers

embed_arweave and embed_text are defined as regular def (not async def), which is actually correct for CPU/IO-bound work in FastAPI (they run in a thread pool). However embed_image is async def and does await file.read() but then immediately calls the synchronous model.encode(img). The model.encode call blocks the event loop. Make it consistent: either make all three sync, or offload model.encode via asyncio.to_thread:

embedding = await asyncio.run_in_executor(None, model.encode, img)

SimilarNfts.tsx — no loading skeleton

When isLoading is true, the component renders NftProvider with loading={isLoading} which presumably shows skeletons — but the section heading "Similar NFTs" is also shown during loading. If loading takes a few seconds, users see a heading with no content beneath it before the skeletons appear. Consider either hiding the heading during loading, or moving the heading inside NftProvider so it appears with the content.

requirements.txt — unpinned Pillow

Pillow>=11.0.0

All other dependencies are pinned to exact versions. Pillow>=11.0.0 allows any future major version bump that may introduce breaking changes or security regressions. Pin to a specific version (e.g., Pillow==11.2.1) for reproducible builds.

Missing Tests / Documentation

• No tests for server.py (unit or integration). At minimum, a test verifying that non-image Arweave IDs are rejected and that the size limit is enforced would be valuable.
• No .env.example or documentation update noting the new REACT_APP_EMBEDDING_SERVER variable.
• No Dockerfile or deployment notes for the new microservice. How is it expected to be run? Are there GPU requirements for CLIP?

Summary

The feature concept is solid and the two-phase lookup (canister fast-path → server fallback) is a nice design. Addressing the N+1 canister calls and the streaming size cap are the two items I'd consider blocking before merge.